-
Notifications
You must be signed in to change notification settings - Fork 22
fix(ToolGuard): updates #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…OAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 3ac6e7a I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: ec34c8b I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 7e77578 I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 86d252f Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 3ac6e7a I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: ec34c8b I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 7e77578 I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 86d252f I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 91936c9 Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
|
Don't forget to signoff on the last two commits or add a remediation commit. |
Signed-off-by: David Boaz <boazdavid@users.noreply.github.com>
I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: ec005bc Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
jsntsay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests also don't work for me. At least when I changed them to use mistral on WX.
Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
jsntsay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed an important change the first time. Should be good to go after that.
Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are broken due to the core llm interface being changed without the proper refactoring. Please revert the core interface change and lets talk about why it's needed. Also in the future please run all unit tests and do DCO for the final PR update.
altk/core/llm/base.py
Outdated
| """ | ||
|
|
||
| @abstractmethod | ||
| def get_model_id(self) -> str|None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this breaks a bunch of tests, need to go through and implement this function for anything that uses this like the various tests/core tests. Please revert this change, I would prefer not changing core interfaces without discussing first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I added the model_id() to support mellea requirement. I consulted with Osher, and we did the changes togeter.
But actually, after your comment, I tested and I found that mellea can afford also without the model_id. so I reverted these changes. sorry.
|
|
||
| specs = cast( | ||
| ToolGuardSpecs, | ||
| await toolguard_spec.aprocess( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test for me fails here:
self = <altk.pre_tool.toolguard.llm_client.TG_LLMEval object at 0x14e6e3210>
llm_client = <altk.core.llm.providers.ibm_watsonx_ai.ibm_watsonx_ai.WatsonxLLMClientOutputVal object at 0x14e6d5950>
def __init__(self, llm_client: Union[LLMClient, ValidatingLLMClient]):
> super().__init__(llm_client.get_model_id()) # type: ignore
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E TypeError: object.__init__() takes exactly one argument (the instance to initialize)
altk/pre_tool/toolguard/llm_client.py:10: TypeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is green for me
Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: ef00372 I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 3f8f9fb I, DAVID BOAZ <DAVIDBO@il.ibm.com>, hereby add my Signed-off-by to this commit: 4ea9d5f Signed-off-by: DAVID BOAZ <DAVIDBO@il.ibm.com>
|
|
||
| #Toolguarg code generation | ||
| build_output = cast(ToolGuardsCodeGenerationResult, | ||
| await toolguard_code.aprocess(input, AgentPhase.BUILDTIME)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New issue running this unit test:
tests/pre_tool/toolguard/test_toolguard_code.py:137:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
altk/core/toolkit.py:109: in aprocess
return await self._abuild(data)
^^^^^^^^^^^^^^^^^^^^^^^^
altk/pre_tool/toolguard/toolguard_code_component.py:81: in _abuild
return await generate_guards_from_specs(
.venv/lib/python3.11/site-packages/toolguard/buildtime.py:99: in generate_guards_from_specs
return await generate_toolguards_from_functions(
.venv/lib/python3.11/site-packages/toolguard/gen_py/gen_toolguards.py:38: in generate_toolguards_from_functions
return await generate_toolguards_from_domain(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
app_name = 'myapp'
specs = [ToolGuardSpec(tool_name='divide_tool', policy_items=[ToolGuardSpecItem(name='Division by Zero is Not Allowed', descri... ToolGuardSpec(tool_name='subtract_tool', policy_items=[]), ToolGuardSpec(tool_name='map_kdi_number', policy_items=[])]
py_root = PosixPath('/Users/jasontsay/workrepos/agent-lifecycle-toolkit/tests/pre_tool/toolguard/outputs/work_20260108_163042')
domain = RuntimeDomain(app_name='myapp', toolguard_common=FileTwin(file_name=PosixPath('rt_toolguard/data_types.py'), content='...().copy()\n args.pop(\'self\', None)\n return self._delegate.invoke(\'map_kdi_number\', args, float)\n'))
llm = <altk.pre_tool.toolguard.llm_client.TG_LLMClient object at 0x1182aaed0>
async def generate_toolguards_from_domain(
app_name: str,
specs: List[ToolGuardSpec],
py_root: Path,
domain: RuntimeDomain,
llm: I_TG_LLM,
) -> ToolGuardsCodeGenerationResult:
# Setup env
pyright.config(py_root)
pytest.configure(py_root)
for tool_policy in specs:
for policy in tool_policy.policy_items:
policy.name = policy.name.replace(".", "_")
not_empty_specs = [
spec
for spec in [
ToolGuardSpec( # a copy
tool_name=spec.tool_name,
policy_items=[i for i in spec.policy_items if not i.skip],
)
for spec in specs
]
if len(spec.policy_items) > 0
]
# mellea_workaround = {"model_options": {"reasoning_effort": "medium"}}#FIXME https://github.com/generative-computing/mellea/issues/270
# kw_args = llm.kw_args
# kw_args.update(mellea_workaround)
> mellea_backend = SimpleBackend(llm)
^^^^^^^^^^^^^^^^^^
E TypeError: Can't instantiate abstract class SimpleBackend with abstract method _generate_from_raw
.venv/lib/python3.11/site-packages/toolguard/gen_py/gen_toolguards.py:89: TypeError
Hard for me to debug since it seems to be an issue within toolguard. For whatever reason I think the wrong llm object is being passed to SimpleBackend(llm) for mellea. I have toolguard 0.1.18 installed.
From looking into it more, I'm assuming it's because I have an older version of mellea installed (we previously fixed it to ~0.0.6) and SimpleBackend relies on a newer version of the ABC here. Even if it is its own repo, I highly suggest fixing specific versions (i.e. >0.1.0 etc.) to each core dependency in the toolguard repo to prevent this sort of thing from happening again.
No description provided.